Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Registries report which commands they support. #5915

Closed
wants to merge 3 commits into from

Conversation

withoutboats
Copy link
Contributor

In the config.json of each registry's index, that registry can report
which commands it supports, under which versions. For example, crates.io's
config.json should look like:

{
    "api": "https://crates.io/",
    "dl": "https://crates.io/api/v1/crates",
    "commands": {
        "publish": ["v1"],
        "yank": ["v1"],
        "owner": ["v1"],
        "search": ["v1"],
        "login": ["v1"]
    }
}

Currently, these five commands only have one version, "v1," but this allows us
to release new, breaking changes to these commands' interface with crates.io
without disrupting the operation of any alternative registry.

Before this is merged, crates.io's config.json will need to be updated, and we
should make an effort to inform known maintainers of alternative registries
that they need to update their indices as well.

This also involved rewriting the initial stage of the cargo login flow to be a
bit more clear.

In the `config.json` of each registry's index, that registry can report
which commands it supports, under which versions. For example, crates.io's
config.json should look like:

```json
{
    "api": "https://crates.io/",
    "dl": "https://crates.io/api/v1/crates",
    "commands": {
        "publish": ["v1"],
        "yank": ["v1"],
        "owner": ["v1"],
        "search": ["v1"],
        "login": ["v1"]
    }
}
```

Currently, these five commands only have one version, "v1," but this allows us
to release new, breaking changes to these commands' interface with crates.io
without disrupting the operation of any alternative registry.

Before this is merged, crates.io's config.json will need to be updated, and we
should make an effort to inform known maintainers of alternative registries
that they need to update their indices as well.
@rust-highfive
Copy link

r? @alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)

@bors
Copy link
Contributor

bors commented Aug 25, 2018

☔ The latest upstream changes (presumably #5939) made this pull request unmergeable. Please resolve the merge conflicts.

@alexcrichton
Copy link
Member

Looks reasonable to me!

@sfackler as the other "co champion" of custom registries, do you have thoughts on this?

Some(registry) => registry,
None => "crates-io",
};
Err(format_err!("`{}` does not support the `cargo login` command with \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should only check this if the token wasn't provided up front right?

@sfackler
Copy link
Member

LGTM other than making sure that cargo login --registry foo --token bar works even if the registry doesn't support the login endpoint.

@withoutboats
Copy link
Contributor Author

@sfackler why should that work? If the registry doesn't support the v1 login flow, that wouldn't be meaningful. If they do support it, they should report that they do.

Imagine that we implement a new login flow in which --token is not meaningful, and the registry only supports that flow. Wouldn't we want that to error?

@sfackler
Copy link
Member

What's the scope of the v1 login flow? Part of the v1 publish flow is that an Authorization header containing the auth token will be sent along with the request. Even if your custom registry server doesn't expose a /login or /me endpoint it can still support publishing. For example, the auth for our internal publishes is granted to individual builds so they know up front what token to use without any explicit login flow. We use cargo login --registry foo --token $MY_CREDENTIALS as a nicer form of echo "[registry.foo]\ntoken = $MY_CREDENTIALS" >> ~/.cargo/credentials.

@withoutboats
Copy link
Contributor Author

withoutboats commented Aug 28, 2018

That's a good question, and I see why you would think your registry doesn't support the v1 flow. I do just think it means storing an API token and passing it with the Authorization header, which is why for registries other than crates.io the message printed to stdout is modified to be less certain about the /me endpoint:

please paste the API Token below (you may be able to obtain your token by visiting https://$URL/me)

This should also be changed so that registries that don't assert they support a login flow but do support other commands are sent requests without any auth header (such registries are presumably relying on being run on an intranet, not saying its a good idea but its the semantic result of saying you support publish but not login)

@withoutboats
Copy link
Contributor Author

This feature is blocking custom registries landing, which is unfortunate. The main blocker on the PR has been that I have a lot of difficulty understanding how to cargo's test suite actually sets up the environment, so I've never been able to get CI to pass.

@alexcrichton
Copy link
Member

@withoutboats sorry for the late reply, but could I help out perhaps with getting cargo test working locally for you? Do you have some example error logs which I could help debug?

@ehuss
Copy link
Contributor

ehuss commented Feb 14, 2019

Closing per discussion in #6442.

@ehuss ehuss closed this Feb 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants